Logging foundation: structured LogEvent, JUL handler, Log API enhancements - #12694

Well-designed foundational logging infrastructure. The structured LogEvent API, JUL handler, and Log API enhancements provide a solid base for the build report and console modes PRs. A few issues noted below.

Also noted:

• Good catch fixing warn(Supplier<String>, Throwable) calling logger.info() instead of logger.warn().
• The logger name change from getFullGoalName() to getImplementation() (FQCN) enables proper hierarchical SLF4J level configuration but is a behavioral change — worth mentioning in release notes for users who configured logging by short-form names.
• No unit tests were added for the new functionality (MavenJulHandler, DefaultLogEvent, StackWalker metadata capture, LogSink contract). Given this is foundational for the entire logging pipeline, targeted tests would increase confidence.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

Really solid foundation — the three-path convergence (Log API / SLF4J / JUL) onto one structured LogEvent is clean, and preserving the LogRecord metadata the stock SLF4JBridgeHandler drops is a genuine improvement. Nice catch on the warn(Supplier, Throwable) → logger.info() bug.

A few things worth a look before this becomes the base of the 7-PR chain — one API-compat question, one fork-context correctness question, one perf note, and some small nits. Nothing structural.

On tests (echoing the earlier note): the two I'd most want are a regression test asserting warn(Supplier, Throwable) actually logs at WARN, and a table test for the JUL→SLF4J level mapping (esp. FINEST→TRACE and CONFIG→INFO). Given the ThreadLocal/StackWalker plumbing, those would lock down the easy-to-regress bits.

Well-designed logging infrastructure foundation with clean three-path convergence (Log API, JUL, SLF4J). The bug fix for warn(Supplier, Throwable) calling logger.info() is confirmed correct.

Findings:

1. [medium] sequenceNumber() javadoc/contract mismatch — LogEvent.sequenceNumber() javadoc says @return the sequence number, always non-negative but the default implementation returns -1. The sibling method threadId() correctly documents or -1 if unavailable in its @return tag. The sequenceNumber() javadoc should follow the same pattern for consistency.

2. [medium] Inconsistent formattedMessage format between JUL and SLF4J — When a LogSink is installed, JUL events' formattedMessage is built by formatForConsole() which produces a minimal [LEVEL] message string, while SLF4J events produce a full formatted string with timestamps, thread names, and logger names via MavenBaseLogger.innerHandleNormalizedLoggingCall(). The practical impact is limited since the clean message() field is available for consumers who need consistent content, but in SimpleBuildEventListener.projectLogMessage() which uses formattedMessage() for console output, JUL events will look noticeably different from SLF4J events.

3. [low] Log4j2/Logback backend removal — The removal of Log4j2Configuration, LogbackConfiguration, and the logback-classic dependency means Maven no longer supports these as alternative SLF4J backends. This is intentional for the Maven 4.x logging redesign, but warrants mention in release notes for users who embedded Maven with a custom logging backend.

This review was generated by an AI agent (Claude Code) and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of Guillaume Nodet

Well-architected logging foundation PR. Clean design with proper ThreadLocal management, volatile concurrency handling, and good layering (API → impl → collector). A few items to address:

High severity:

1. API contract contradiction (LogEvent.java line 188): sequenceNumber() Javadoc says "@return the sequence number, always non-negative" but the default implementation returns -1. Compare with threadId() which correctly documents "or -1 if unavailable". This is a public API interface marked @Experimental/@since 4.1.0 — the Javadoc should match the actual contract.

2. No test coverage: 1000+ lines of foundational code across 22 files with zero test files. LogEvent/DefaultLogEvent, MavenJulHandler (249 lines), DefaultLog.withMetadata/trace/child, LogSink interface, ProjectBuildLogAppender structured event creation, and the mojo MDC lifecycle are all untested. The PR description mentions "580 tests pass" but these are all pre-existing tests.

Medium severity:

3. StackWalker overhead (DefaultLog.java line 649): withMetadata() calls StackWalker.walk() on every log call for enabled levels. While trace/debug are typically disabled and info/warn/error are low-volume, plugins logging many INFO/WARN messages will pay the 1-5μs per-call cost.

4. Logger name change (DefaultBuildPluginManager.java line 128): Logger name changed from getFullGoalName() (e.g., "compiler:compile") to getImplementation() (e.g., "org.apache.maven.plugins.compiler.CompilerMojo"). Intentional for proper hierarchical SLF4J configuration, but a user-visible behavior change that could break existing SLF4J level configurations.

5. Dead code for future PR (ProjectBuildLogAppender.java line 130): reportCapture volatile field and setter are infrastructure for PR #12695 (build report). Currently unused in this PR — consider adding a brief comment noting the intent.

Low severity:

6. setMojoId(null) is called before delegate.mojoSucceeded/mojoFailed callbacks, inconsistent with the forkSucceeded/forkFailed pattern where cleanup happens after the delegate.

7. The bug fix changing logger.info() to logger.warn() in warn(Supplier<String>, Throwable) is correct and important. 👍

The removal of Logback/Log4j2 support is a significant architectural decision — worth explicit mention in release notes since users plugging in alternative SLF4J backends will lose that ability.

This review was generated by an AI agent (Claude Code) and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of Guillaume Nodet

Re-review of commit 473a044526 (force-push rebase onto master, 2026-09-18).

Both findings from the previous REQUEST_CHANGES review are confirmed addressed:

1. @param content tag restored — trace(Supplier<String> content) now has @param content the message supplier, consistent with all other supplier-based overloads in the interface.

2. @param content + @param error tags restored — trace(Supplier<String> content, Throwable error) now carries both @param tags, matching the other supplier+throwable overloads.

New code in this commit reviewed and clean:

• LogEvent.projectId() and mojoId() — properly @Nullable-annotated default methods with accurate Javadoc; DefaultLogEvent record components wired correctly in ProjectBuildLogAppender.accept() via MOJO_ID.get().
• DefaultLog.withMetadata() refactor — the fast path (else { logAction.run(); } when report capture is off) correctly avoids all ThreadLocal and StackWalker cost.
• LookupInvoker drain reorg — drain now happens in activateLogging() after createTerminal() has installed ProjectBuildLogAppender; the ordering (drain into new Slf4jLogger before context.logger = logger) is correct. The failOnSeverity message accumulates in the old logger and drains into the new one in the same call — no double-replay risk.
• configureLogging() early SEVERE guard — MavenJulHandler.install() + Level.SEVERE on the JUL root logger in quiet mode is correctly placed before createTerminal() runs.
• MavenJulHandlerTest comment corrected to accurately describe the reentrancy simulation.

Two earlier non-blocking observations remain open (DefaultLog.child(name) blank-name guard; LogEvent.formattedMessage() Javadoc multi-line throwable note) — neither blocks this PR.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review of commit 473a0445 (force-push rebase, 2026-09-18).

Both findings from the CHANGES_REQUESTED review on 6ccbc27a are confirmed fixed:

1. @param content / @param error tags — restored in Log.java for both trace(Supplier<String>) (line 93) and trace(Supplier<String>, Throwable) (line 104). Matches every other supplier overload in the interface. ✅

2. version.* → lifecycle.* property rename — consistent across pom.xml property declarations, plugin-versions.properties keys/values, and PluginVersions.version() key construction. ✅

New additions in this commit:

• Windows quiet-mode JUL fix — configureLogging() now eagerly installs MavenJulHandler and sets JUL root to SEVERE in quiet mode, before createTerminal() runs. This closes the race window where JLine terminal-init JUL events (fired between configureLogging() and activateLogging()) leaked into quiet output. The approach is sound: SLF4J is already bootstrapped at this point (via LoggerFactory.getILoggerFactory() earlier in configureLogging()), so the install doesn't trigger the computeIfAbsent reentrancy flood that installing before SLF4J bootstrap would. activateLogging() idempotently skips re-installation via isInstalled(). ✅

• LogEvent API moved to maven-api-core (org.apache.maven.api.build.report) — LogEvent interface promoted from maven-internal to public API. projectId() and mojoId() default methods added. DefaultLogEvent record updated with matching fields, populated in ProjectBuildLogAppender.accept() from the PROJECT_ID/MOJO_ID ThreadLocals. The LogEvent is now self-contained with execution context without requiring callers to correlate against lifecycle events. ✅

• DefaultLog.withMetadata() optimization — ThreadLocal + StackWalker cost now fully avoided when hasReportCapture() is false. The branch is clean: report-capture path sets and removes the ThreadLocal in try/finally; non-capture path calls logAction.run() directly. ProjectBuildLogAppender.accept() correctly falls through to null source metadata when getLogApiMetadata() returns null (no-capture case). ✅

• pendingEarlyLogs removal — drain now happens directly in activateLogging() after createTerminal() has installed the ProjectBuildLogAppender logSink. The field is gone from LookupContext. Simpler and correct — no more dead-code drain in createTerminal(). ✅

• MavenJulHandlerTest comment fix — comment now accurately describes the test: simulating re-entrance by setting IN_PUBLISH=true directly, rather than the misleading "install a custom SLF4J logger that fires a second JUL event". ✅

One low-severity observation (not blocking):

In configureLogging(), there is a tiny window between MavenJulHandler.install() (which leaves JUL root level at the JUL default, typically INFO) and the subsequent LogManager.getLogManager().getLogger("").setLevel(Level.SEVERE). Any JUL event at INFO or WARNING fired in that two-line gap on a concurrent thread would pass through MavenJulHandler.publish(). In practice this is not exploitable (the startup is effectively single-threaded at this point and no concurrent threads are emitting JUL events yet), but the two lines could be reordered — set level first, then install — to eliminate even the theoretical gap. Not worth a follow-up PR; just noting it for completeness.

APPROVED. Both prior findings addressed, new changes correct.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review of commit 473a0445 (final squash, 2026-09-18T16:59:37Z).

The two @param tags flagged in the previous CHANGES_REQUESTED (on 6ccbc27a) are confirmed present in this squash:

• trace(Supplier<String> content) — @param content the message supplier at line 91 ✅
• trace(Supplier<String> content, Throwable error) — both @param content and @param error at lines 101-102 ✅

All prior findings from previous review cycles are addressed. No new findings.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review of commit 98915c1a (squash-rebase, 2026-09-19).

The CHANGES_REQUESTED finding from the previous review (@param content and @param error tags dropped in squash on trace(Supplier<String>) / trace(Supplier<String>, Throwable)) is confirmed fixed — both tags are present. The other significant changes in this squash are also solid: projectId()/mojoId() propagation through DefaultLogEvent is correct, the drain moved to activateLogging() fixes the dead-code bug cleanly, and Level.SEVERE in quiet mode correctly matches Slf4jConfiguration.Level.ERROR.

Two low-severity issues remain open.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review of commit 98915c1a (force-push squash, 2026-09-19).

Prior CHANGES_REQUESTED resolved: Both @param content tags in Log.java (for trace(Supplier<String>) at line 93 and trace(Supplier<String>, Throwable) at line 104) are present — the squash-rebase regression is fixed.

Two low-priority findings remain from the earlier review thread that were deferred due to the dedup gate firing. Raising them now.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review of commit f68c43db (force-push rebase onto master, 2026-09-20).

All previously requested changes are now resolved: the two @param tags dropped in the 6ccbc27a squash (@param content on trace(Supplier<String>) and @param content/@param error on trace(Supplier<String>, Throwable)) are present in the current HEAD, and the prior low-priority findings from 98915c1a (the message() @return copy-paste and the hollow logApiMetadataIsClearedAfterCall() test) are both fixed.

One new finding below.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review of commit 3f5b4393 ("Address review: call isLoggable() in MavenJulHandler.publish() per Handler contract") and d7f564ab ("Address review: fix LogEvent.message() @return Javadoc, strengthen logApiMetadata test").

Prior finding addressed: ✅ isLoggable(record) is now called in MavenJulHandler.publish() before the re-entrancy guard — correctly honouring the java.util.logging.Handler contract. Placement is right: null check → isLoggable → re-entrancy guard.

New finding (one): The rebase did not correctly pick up master's lifecycle.* property rename, causing the branch to diverge from master on three files.

[high] Bad rebase: branch reverts lifecycle.* rename already on master

Master (current tip) has renamed the lifecycle plugin version properties from version.maven-*-plugin to lifecycle.maven-*-plugin across three files:

• impl/maven-core/pom.xml — property declarations
• impl/maven-core/src/main/resources/org/apache/maven/lifecycle/plugin-versions.properties — property keys
• impl/maven-core/src/main/java/org/apache/maven/lifecycle/PluginVersions.java — key lookup ("lifecycle." + pluginArtifactId)

This branch still has the old version.* names in all three files. When merged, the PR would revert that rename, causing a key mismatch at startup: PluginVersions.java looks up version.maven-clean-plugin, but the properties file (coming from master's version) would have lifecycle.maven-clean-plugin — producing IllegalArgumentException: No default version defined for maven-clean-plugin at class initialisation time.

In addition, the branch carries plugin versions from before the rename (e.g. maven-clean-plugin 3.4.0 vs master's 3.5.0).

The fix is a clean rebase: let git rebase bring in master's version of these three files rather than keeping the branch's old content. These files are unrelated to this PR's stated purpose and should be transparent to the rebase.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review of commits cbc1d608 ("fix: restore master's lifecycle plugin versions in pom.xml (rebase regression)") and bbff94a9 ("fix: update lifecycle plugin versions in pom.xml to match master").

Prior CHANGES_REQUESTED resolved: ✅ The bad-rebase finding from review 5261571175 is fully addressed. All three files called out are now correct:

• impl/maven-core/pom.xml — all 13 lifecycle.maven-*-plugin property values now match master exactly (verified blob-level: PR and master are identical line-for-line)
• impl/maven-core/src/main/resources/org/apache/maven/lifecycle/plugin-versions.properties — same blob hash as master (671212e9f2ce)
• impl/maven-core/src/main/java/org/apache/maven/lifecycle/PluginVersions.java — same blob hash as master (af623cf7d35e)

The version.maven-*-plugin comment in the <pluginManagement> section (line 284) is pre-existing on master and was not introduced by this PR; no action needed there.

The two fix commits are purely mechanical (13 version bumps in pom.xml + 1-line comment alignment) — no logic changes. The core logging files (LogEvent, LogLevel, Log, DefaultLogEvent, MavenJulHandler, MavenBaseLogger, MavenSimpleLogger) are unchanged from the previously approved state.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review of commits cbc1d608db ("fix: restore master's lifecycle plugin versions in pom.xml (rebase regression)") and bbff94a91c ("fix: update lifecycle plugin versions in pom.xml to match master").

Prior CHANGES_REQUESTED finding — confirmed resolved:

The blocking rebase regression flagged on 3f5b4393 is fixed. Both commits restore the lifecycle.maven-*-plugin property names and their current master values across all 13 lifecycle plugin version properties in impl/maven-core/pom.xml:

• maven-clean-plugin: 3.4.0 → 3.5.0 ✅
• maven-compiler-plugin: 3.13.0 → 3.16.0 ✅
• maven-deploy-plugin: 3.1.3 → 3.2.0 ✅
• maven-ear-plugin: 3.3.0 → 3.4.0 ✅
• maven-ejb-plugin: 3.2.1 → 3.3.0 ✅
• maven-install-plugin: 3.1.3 → 3.2.0 ✅
• maven-jar-plugin: 3.4.2 → 3.5.1 ✅
• maven-plugin-plugin: 3.15.1 → 3.16.0 ✅
• maven-rar-plugin: 3.0.0 → 3.1.0 ✅
• maven-resources-plugin: 3.3.1 → 3.5.0 ✅
• maven-site-plugin: 3.21.0 → 3.22.0 ✅
• maven-surefire-plugin: 3.5.2 → 3.6.0 ✅
• maven-war-plugin: 3.4.0 → 3.5.1 ✅

The plugin-versions.properties and PluginVersions.java key scheme (lifecycle.*) is consistent with the updated POM properties. The ExceptionInInitializerError at class load time will no longer occur. The pluginManagement comment saying "version.maven-*-plugin" is a pre-existing stale comment on master (not introduced by this PR) — not blocking.

All prior findings across the full review history remain addressed. No new issues.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review of commits cbc1d608db ("fix: restore master's lifecycle plugin versions in pom.xml (rebase regression)") and bbff94a91c ("fix: update lifecycle plugin versions in pom.xml to match master").

Prior CHANGES_REQUESTED finding — resolved.

The rebase regression that introduced old lifecycle plugin versions (clean/3.4.0, compiler/3.13.0, etc.) is fully fixed. All three affected files now match master exactly:

• ✅ impl/maven-core/pom.xml — all 13 lifecycle.maven-*-plugin properties updated to master versions (clean/3.5.0, compiler/3.16.0, deploy/3.2.0, ear/3.4.0, ejb/3.3.0, install/3.2.0, jar/3.5.1, plugin/3.16.0, rar/3.1.0, resources/3.5.0, site/3.22.0, surefire/3.6.0, war/3.5.1). Blob diff vs master: none.
• ✅ plugin-versions.properties — identical blob to master (671212e9). ${lifecycle.maven-*-plugin} filter references are correct.
• ✅ PluginVersions.java — identical blob to master. No ExceptionInInitializerError risk at class init.

All core logging foundation files (LogEvent, LogLevel, Log, DefaultLogEvent, DefaultLog, MavenJulHandler, MavenBaseLogger, MavenSimpleLogger, ProjectBuildLogAppender, SimpleBuildEventListener) are unchanged from the previously APPROVED state at 3f5b4393.

CI is pending — no functional risk identified in this incremental delta.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review of commit 707b3555 (squash rebase onto master, 2026-09-20).

Three correctness regressions in this squash. The logging foundation code itself (LogEvent API, MavenJulHandler, ProjectBuildLogAppender restructuring, DefaultLog metadata) is solid and unchanged from the previously APPROVED state. The issues are all in the bundled fixes.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review of squash commit 707b3555 (force-push rebase onto master, 2026-09-20).

All prior findings addressed in this squash:

1. @param content missing on Supplier-based trace() methods (CHANGES_REQUESTED on 6ccbc27a) — fixed: @param content the message supplier now present on both trace(Supplier<String>) and trace(Supplier<String>, Throwable).
2. LogEvent.message() @return copy-paste — fixed: @return the log message, never {@code null} now matches the body description.
3. pendingEarlyLogs dead drain in createTerminal() (raised on 3752425804) — resolved with a cleaner approach: activateLogging() now drains directly into the SLF4J logger after createTerminal() has already installed ProjectBuildLogAppender. No pendingEarlyLogs field needed.
4. Quiet-mode JUL root level WARNING → SEVERE — fixed in configureLogging(), correctly using Level.SEVERE for the quiet-mode pre-guard.
5. isLoggable(record) in MavenJulHandler.publish() — present, correct.
6. IN_PUBLISH reentrancy guard — present with correct try/finally cleanup.

One low-severity finding on the reentrancy test.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review of commits 2272eae465 ("fix: address review — restore concurrency guard in MavenProject.getArtifacts(), cleanup in finally, ancestor-dir guard in PluginUpgradeStrategy") and b0a1155e82 ("Address review: replace IN_PUBLISH reflection in test with package-private hook").

All four outstanding findings are confirmed resolved:

1. ✅ MavenProject.getArtifacts() partial-set visibility — intermediate result variable restored; artifacts field is now assigned atomically after the filter loop completes, eliminating the concurrent partial-set observation window introduced by the squash.

2. ✅ PluginUpgradeStrategy.doApply() tempDir leak on exception — tempDir declared before the try, cleanup moved to finally with a null guard. Any exception path (including createTempProjectStructure() itself) now reliably triggers cleanup.

3. ✅ upgradePropertyVersion() sibling/child POM mutation — ancestor-directory guard restored: currentDir.startsWith(candidateDir) correctly restricts the cross-POM property search to ancestor directories only (a directory is an ancestor iff the current directory starts with its path).

4. ✅ publishIsReentrantSafe() reflection brittleness — setInPublishForTest(boolean) package-private hook added. Test and production code are in the same package (org.apache.maven.slf4j), so the hook is accessible without reflection. The test is also meaningfully simplified — unnecessary root-handler save/restore noise eliminated, FQCN replaced with proper import.

One trivial nit (informational): The setInPublishForTest Javadoc says @param inPublish ... {@code false} (or pass {@code null} via the {@code remove} path) — but the parameter is a primitive boolean, so null cannot be passed. The remove() semantics are an implementation detail of the false branch, not a separate call convention. The Javadoc is harmless but slightly misleading; worth fixing in a follow-up if there is one.

No new issues found. This PR is ready to merge.

This review was generated by an AI agent, Hermès.

Re-review of commits 2272eae4 ("fix: address review — restore concurrency guard, cleanup in finally, ancestor-dir guard") and b0a1155e ("Address review: replace IN_PUBLISH reflection in test with package-private hook").

All three CHANGES_REQUESTED findings from 707b3555 are confirmed addressed:

1. Concurrency regression in MavenProject.getArtifacts() — ✅ Restored. The local result variable is back; artifacts is only assigned after the loop completes. Concurrent readers can no longer observe a partially-populated set. The fix is identical to the original pre-squash code.

2. Temp-directory leak in PluginUpgradeStrategy.doApply() — ✅ Fixed. tempDir is now declared before the try block (initialized to null), and cleanupTempDirectory() is in a finally with a null guard. Any uncaught exception between createTempProjectStructure() and the old cleanup call no longer leaks the temp directory.

3. Sibling-POM mutation regression in upgradePropertyVersion() — ✅ Fixed. The ancestor-directory guard is restored: currentPomPath/currentDir are resolved from the pomMap, and the per-candidate check !currentDir.startsWith(candidateDir) skips non-ancestor POMs. Sibling and child POMs are correctly excluded from cross-POM property searches.

b0a1155e — IN_PUBLISH reflection replaced with package-private hook:

The refactor is clean. MavenJulHandler.setInPublishForTest(boolean) is package-private (same package as the test), has accurate Javadoc, and correctly uses IN_PUBLISH.set(Boolean.TRUE) / IN_PUBLISH.remove() mirroring what publish() does internally. The test is simplified: the unnecessary JUL root-handler save/restore scaffolding is gone, the throws Exception declaration is removed, and the assertion is now self-documenting. If IN_PUBLISH is ever renamed, the test will fail at compile time rather than at runtime with NoSuchFieldException.

No new issues. All prior findings from the full review history are addressed. The logging foundation is solid and ready.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review of commit 218c57feec ("fix: remove extra blank line to stay within 2000-line checkstyle limit").

Single-line cosmetic change — removes the blank line between this.artifacts = artifacts; and // flush the calculated artifactMap in setArtifacts(). No logic change.

Prior APPROVE on b0a1155e82 stands. PR is ready to merge.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review of commit 218c57fe (force-push rebase onto master, 2026-09-20).

Two real issues found. The previous CHANGES_REQUESTED on @param tags is resolved — both @param content the message supplier and @param error the error that caused this log are present in Log.java lines 91–104 of this commit. The logging foundation code (MavenJulHandler, ProjectBuildLogAppender, DefaultLog, LogEvent API) is unchanged from the previously approved state.

Finding 1 (high): PomInlinerTransformer — CI-friendly version regression re-introduced
Finding 2 (medium): PluginUpgradeStrategy.analyzePluginsUsingEffectiveModels — findCommonRoot called per-module inside the loop (O(n²))

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review of commit 628c65fa ("Address review: restore GH-13192 PomInliner pomProperties fallback, hoist findCommonRoot out of loop").

All four CHANGES_REQUESTED findings from the previous review (707b3555) are confirmed fixed:

1. ✅ MavenProject.getArtifacts() concurrency guard — local result variable restored; artifacts = result assigned atomically after the loop. No concurrent reader can observe a partially-populated set.
2. ✅ PluginUpgradeStrategy temp-dir leak — cleanupTempDirectory() moved to finally block; cleanup is guaranteed even when an exception is thrown.
3. ✅ Sibling-POM mutation guard — ancestor-directory restriction (currentDir.startsWith(candidateDir)) correctly limits cross-POM property searches to parent directories only.
4. ✅ MavenJulHandlerTest reflection — replaced with package-private setInPublishForTest() hook; test no longer breaks on IN_PUBLISH field rename.

Also confirmed in this squash:

• ✅ findCommonRoot hoisted out of the per-module loop — O(N²) → O(N).
• ✅ MavenJulHandler.publish() now calls isLoggable(record) per the Handler contract.
• ✅ PomInlinerTransformer CI-friendly fallback: GAV-keyed pomProperties cache correctly propagates POM-defined ${revision} through injectTransformedArtifacts → replacePom without cross-module collision. IT test MavenITgh13192PomInlinerCiFriendlyPropertyTest covers the regression.

One gap: missing negative test for the sibling-mutation guard.

upgradePropertyVersion() has an ancestor-directory guard (!currentDir.startsWith(candidateDir)) that prevents sibling POMs from being mutated when they happen to define the same property. The existing shouldUpgradePluginWithPropertyVersionInParentPom test covers the success path (parent gets upgraded) and shouldNotWarnWhenPropertyAlreadyAtTargetVersion covers the no-change path, but there is no test that verifies a sibling POM at the same directory level is not mutated.

Without this test, a future refactor that removes the guard will silently regress to the original bug. The scenario to add:

• root/pom.xml — no exec.maven.version property
• root/module-a/pom.xml — defines <exec.maven.version>3.1.0</exec.maven.version>, uses ${exec.maven.version}
• root/module-b/pom.xml — also defines <exec.maven.version>3.1.0</exec.maven.version> independently

After doApply: module-a’s property upgraded to 3.5.0. Module-b’s property must remain untouched (it was not the target of the upgrade).

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review of commits 707b355→628c65fa (force-push rebase, 2026-09-20).

All prior findings are addressed:

• @param content / @param error tags (CHANGES_REQUESTED, 2026-09-18): restored in Log.java — both trace(Supplier<String>) and trace(Supplier<String>, Throwable) now have correct @param tags. ✓
• Concurrency guard in MavenProject.getArtifacts(): result local variable restored — artifacts is only assigned after the loop completes. ✓
• IN_PUBLISH reflection in test: replaced with the new setInPublishForTest(boolean) package-private hook. ✓
• cleanupTempDirectory() in finally block: confirmed present in PluginUpgradeStrategy.doApply(). ✓
• Quiet-mode JUL root level SEVERE: confirmed in LookupInvoker.configureLogging() and activateLogging(). ✓

Two remaining gaps:

1. cleanupTempDirectory() leaks the Files.walk() stream (new finding)
2. MavenProjectGetArtifactsTest deleted without replacement (regression protection gap)

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review of commit a471a464 (rebase onto master, 2026-09-20).

The prior CHANGES_REQUESTED finding from 6ccbc27a is confirmed addressed:

• @param content / @param error tags — both trace(Supplier<String>) and trace(Supplier<String>, Throwable) carry their @param tags in the current head. The squash-rebase regression is gone.

The new commit also restores .github/workflows/update-lifecycle-plugins.yml and the versions-update profile in pom.xml — infrastructure that was merged to master in #13183/#13186 and dropped during the previous rebase. No issues with those additions.

Prior APPROVE stands. No new findings.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review of commit 319c96d7 ("fix: restore AbstractUpgradeStrategy reactor pre-build and related mvnup classes from master").

This commit restores verbatim from master the prebuildReactorModels/effectiveModelCache/sharedModelBuilderSession machinery dropped during a prior rebase (introduced by #13190). No logging foundation code was changed.

Verified:

• @param content and @param error tags on trace(Supplier<String>) and trace(Supplier<String>, Throwable) in Log.java — both present (lines 91-104). The regression from 6ccbc27a is fixed.
• prebuildReactorModels() correctness: root POM identified via getNameCount() minimum on absolute paths — correct. Cache keys use toAbsolutePath().normalize() consistently on both write (line 209) and read (line 441) sides — no path-mismatch risk.
• sharedModelBuilderSession fallback in buildEffectiveModel() when cache misses — correct; lazily created and reused so mappedSources from the reactor pre-build is available for external parent resolution.
• State cleanup in finally block of apply() — both effectiveModelCache and sharedModelBuilderSession nulled out; singleton instances cannot leak state across invocations.
• ToolchainPluginStrategy: decoupled from running JDK (getRunningJdkMajor() hook removed), now purely declaration-based via latestJdkForSourceLevel() — correct per the stated intent (act on what the project declares, not what JDK is running).
• AbstractUpgradeStrategyTest: solid regression test for #13190 — multi-module project with <version> omitted in child POM, verifying inference works without "version is missing" cascade.
• @param removals in AbstractUpgradeGoal protected methods — verbatim from master, consistent with the class being an internal implementation detail.

No new issues. Prior APPROVE stands.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review of commits 9d628ee9 ("fix: restore out-of-scope test changes from master"), cfa0ff42 ("fix: restore ToolchainPluginStrategyTest from master"), and 1d9c90d8 ("fix: restore out-of-scope changes in ExecutionEventLogger, Log.java, AbstractMavenTransferListener, DefaultModelBuilder").

Prior CHANGES_REQUESTED finding — resolved:

• ✅ @param content and @param error tags on trace(Supplier<String>) and trace(Supplier<String>, Throwable) in Log.java — both present in the current branch.

New commits reviewed:

All three commits are pure restoration of out-of-scope code to match master — no logging-foundation logic was touched:

• 9d628ee9 / cfa0ff42: test-only restores (PluginUpgradeStrategyTest, ParentCycleDetectionTest, ToolchainPluginStrategyTest) — correct.
• 1d9c90d8 ExecutionEventLogger: restores group=2 for unknown build status (correct — logReactorSummaryGroup is called for groups 0, 1, and 2) and removes the now-unused buffer field from ReactorSummaryRequest. Both changes match master exactly.
• 1d9c90d8 AbstractMavenTransferListener: removes redundant inline field/constructor Javadoc — fine.
• 1d9c90d8 DefaultModelBuilder: adds a better error hint for <relativePath> mismatch — correct and helpful.
• 1d9c90d8 Log.java: Javadoc improvements for trace, debug, and child() — accurate and cleaner.

Prior low nit (test comment in publishIsReentrantSafe()): Addressed — the comment now correctly describes the test hook mechanism.

CI is pending (initial-build + Jenkins). Logging foundation code itself is unchanged from the previously approved state.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review of commit 1d9c90d (force-push rebase onto master, 2026-09-20).

The two findings from the previous REQUEST_CHANGES review are confirmed addressed:

1. @param content tag restored on trace(Supplier<String>) — present at line 93: @param content the message supplier. ✅
2. @param content and @param error tags restored on trace(Supplier<String>, Throwable) — both present at lines 101-102. ✅

Three additional commits bundled in this push:

• 2272eae (fix: address review — restore concurrency guard in MavenProject.getArtifacts()): correct — builds into a local result set before assigning artifacts = result within the synchronized block, preventing a half-populated field from being visible to concurrent readers during filtering. ✅
• 628c65f (fix: restore GH-13192 PomInliner pomProperties fallback, hoist findCommonRoot out of loop): out-of-scope changes correctly restored from master. ✅
• b0a1155 (Address review: replace IN_PUBLISH reflection in test with package-private hook): test comment and implementation updated correctly — publishIsReentrantSafe now uses setInPublishForTest() hook and the comment accurately describes the scenario being tested. ✅

Remaining low-severity observations from the prior APPROVE (not blocking):

• Thread.currentThread().getId() is deprecated since Java 19 — @SuppressWarnings("deprecation") is present but Thread.threadId() would be cleaner.
• DefaultLog.child() documents "must not be null or blank" but DefaultLog only checks requireNonNull — no blank guard.
• LogEvent.formattedMessage() Javadoc doesn't mention multi-line throwable rendering when an exception is present.

None of these block merge.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review of squash commit abd26c76 ("feat: logging foundation – structured LogEvent, JUL handler, Log API enhancements", 2026-09-20).

Both findings from the previous REQUEST_CHANGES review are confirmed addressed:

1. @param content tag on trace(Supplier<String> content) — present at line 91 of Log.java, consistent with all other supplier-based overloads. ✅
2. @param content / @param error tags on trace(Supplier<String>, Throwable) — both present at lines 101-102. ✅

Also confirmed in this squash:

• sequenceNumber() @return correctly reads or {@code -1} if unavailable (no longer "always non-negative").
• LogEvent.message() @return javadoc copy-paste from formattedMessage() fixed.
• logApiMetadataIsClearedAfterCall() test strengthened to exercise the remove() path.

All previously raised findings are resolved. Prior approval stands.

This review was generated by an AI agent, Hermès on behalf of @gnodet.